Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add namespace to create environment form #361

Merged
merged 6 commits into from
Mar 20, 2024

Conversation

gabalafou
Copy link
Contributor

Fixes one half of #307.

Description

This pull request shows the namespace on the form/tab to create a new environment.

Pull request checklist

  • Did you test this change locally?
  • Did you update the documentation (if required)?
  • Did you add/update relevant tests for this change (if required)?

@gabalafou
Copy link
Contributor Author

I'm not sure this looks the way we want:

screenshot

Side comment: in that screenshot I'm using the MUI TextField component in disabled mode. That seemed like the most straightforward way to add the namespace to the page. However, the colors on disabled text fields in MUI are low contrast, so I think I should probably re-implement the namespace as something else.

@gabalafou
Copy link
Contributor Author

@smeragoel when you get a chance could you review this?

@peytondmurray peytondmurray self-requested a review March 5, 2024 21:00
Copy link
Contributor

@peytondmurray peytondmurray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good - see comments though.

it("should render component in create mode with namespace", () => {
const mockOnUpdateName = jest.fn();
const component = render(
mockTheme(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my own understanding: why use mockTheme instead of <MockTheme> for that component here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. Two things:

  1. I'm not super familiar with MUI or testing MUI. I was just patterning after what's already in this test file.
  2. The function mockTheme is not written as a functional React component, but it could be.

The utility function is so lightweight that I'm thinking it might be better to replace the call to mockTheme() everywhere with:

import { condaStoreTheme } from "../src/theme";
// ...
<ThemeProvider theme={condaStoreTheme}>
  <Provider store={store}>
    <ComponentUnderTest />
  </Provider>
</ThemeProvider>

@peytondmurray peytondmurray added area: UI design 🎨 Items related to the user interface area: user experience 👩🏻‍💻 Items impacting the end-user experience and removed needs: review 👀 labels Mar 6, 2024
@smeragoel
Copy link

@gabalafou and I discussed this issue and we decided that we're gonna see how much effort it is to customize the input component to match the designs and then we can decide the way forward

@gabalafou
Copy link
Contributor Author

Given the conversation in today's planning meeting, I reverted my last commit in favor of using the MUI TextField component without customization

Copy link
Contributor

@peytondmurray peytondmurray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good - thanks for adding that aria-hidden property. Can you make sure the playwright tests are okay before merging?

@gabalafou
Copy link
Contributor Author

The Playwright test is probably a legit failure. Need to investigate.

@gabalafou gabalafou merged commit ee80e93 into conda-incubator:main Mar 20, 2024
4 checks passed
@gabalafou
Copy link
Contributor Author

Merged since all checks pass!

@gabalafou gabalafou deleted the namespace-create-env branch March 20, 2024 14:43
gabalafou added a commit to gabalafou/conda-store-ui that referenced this pull request Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: UI design 🎨 Items related to the user interface area: user experience 👩🏻‍💻 Items impacting the end-user experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants